ui: thousands separators on delta percentages#4
Merged
Conversation
The four LICENSE files (repo root and the three SDKs) carried real Apache-2.0, but the text had been edited: a leading blank line, the APPENDIX section removed, and a hardcoded "Copyright 2026 Oddbit" spliced in place of the placeholder copyright notice. pana matches LICENSE files against the SPDX corpus; once the deltas exceed its threshold it returns "No license was recognized", which cost the Dart SDK the full 10 points on pub.dev's OSI-approved-license check (visible at 150/160 on shrtnr 1.0.1). Replaced all four files with the canonical text from https://www.apache.org/licenses/LICENSE-2.0.txt (sha256 cfc7749b96f63bd31c3c42b5c471bf756814053e847c10f3eb003417bc523d30). Copyright and attribution already live in each SDK's NOTICE file, so they don't need to be embedded in LICENSE. npm and PyPI don't run pana; the change to those LICENSE files is parity hygiene, not a packaging fix, so no version bump is needed there. The Dart SDK gets a 1.0.2 patch in a follow-up commit so pub.dev re-scores the package.
Patch bump only. Picks up the LICENSE normalization from the previous commit so pana recognizes Apache-2.0 and restores the missing 10 points on the OSI-approved-license check. No public surface or dependency changes; spec hash is unchanged.
The Delta component rendered raw integers (e.g. "+1400%"), which is awkward
once a delta exceeds three digits. Pipe the page's lang through KpiCard and
through every Delta call site (dashboard KPIs, links list, bundles list) so
the percentage runs through fmtNumber() and renders with the locale's
thousands grouping ("+1,400%" in en).
Test added: links-page asserts a 4-digit delta surfaces as "+1,400%" and never
as the bare "+1400%".
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
shrtnr | 508ec18 | May 07 2026, 05:47 AM |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates delta percentage rendering to be locale-aware (adding thousands separators via Intl.NumberFormat) and threads lang through components; also includes SDK licensing and packaging metadata updates.
Changes:
- Format
Deltapercentage values viafmtNumber(pct, lang)and propagatelangthroughKpiCardand page call sites. - Add a regression test ensuring
+1400%renders as+1,400%(defaulten). - Update LICENSE files (root + SDKs) and bump Dart SDK version/changelog.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/links.tsx | Passes lang into Delta for locale-aware pct formatting. |
| src/pages/dashboard.tsx | Passes lang into KpiCard so KPI deltas can be localized. |
| src/pages/bundles.tsx | Passes lang into Delta for bundle card deltas. |
| src/components/kpi-card.tsx | Makes lang a required prop and forwards it to Delta. |
| src/components/delta.tsx | Localizes pct display using fmtNumber(pct, lang). |
| src/tests/page/links-page.test.ts | Adds regression coverage for thousands separators in delta pcts. |
| sdk/typescript/LICENSE | Adjusts Apache-2.0 license text (intended canonicalization). |
| sdk/python/LICENSE | Adjusts Apache-2.0 license text (intended canonicalization). |
| sdk/dart/LICENSE | Adjusts Apache-2.0 license text (intended canonicalization). |
| sdk/dart/pubspec.yaml | Bumps Dart SDK version to 1.0.2. |
| sdk/dart/CHANGELOG.md | Documents Dart SDK license change in 1.0.2 entry. |
| LICENSE | Adjusts root Apache-2.0 license text (intended canonicalization). |
Comments suppressed due to low confidence (3)
LICENSE:1
- If the intent is to use the canonical Apache-2.0 text for license scanners, these edits likely move away from the canonical wording (e.g., the official text uses “submitted to the Licensor …” / “received by the Licensor …” and typically “excluding any notices …”). Consider replacing the full LICENSE content with an exact copy from apache.org (no wording tweaks) to avoid tooling failing to recognize the license. Apply the same fix consistently to
sdk/typescript/LICENSE,sdk/python/LICENSE, andsdk/dart/LICENSE.
LICENSE:1 - If the intent is to use the canonical Apache-2.0 text for license scanners, these edits likely move away from the canonical wording (e.g., the official text uses “submitted to the Licensor …” / “received by the Licensor …” and typically “excluding any notices …”). Consider replacing the full LICENSE content with an exact copy from apache.org (no wording tweaks) to avoid tooling failing to recognize the license. Apply the same fix consistently to
sdk/typescript/LICENSE,sdk/python/LICENSE, andsdk/dart/LICENSE.
LICENSE:1 - If the intent is to use the canonical Apache-2.0 text for license scanners, these edits likely move away from the canonical wording (e.g., the official text uses “submitted to the Licensor …” / “received by the Licensor …” and typically “excluding any notices …”). Consider replacing the full LICENSE content with an exact copy from apache.org (no wording tweaks) to avoid tooling failing to recognize the license. Apply the same fix consistently to
sdk/typescript/LICENSE,sdk/python/LICENSE, andsdk/dart/LICENSE.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: shrtnr | ||
| description: Dart client for the shrtnr URL shortener API. Create short links, manage custom slugs, and read click analytics. | ||
| version: 1.0.1 | ||
| version: 1.0.2 |
Member
Author
There was a problem hiding this comment.
Good catch. Updated the PR description to call out the LICENSE canonicalization (06645da) and Dart 1.0.2 bump (f72e3e2) alongside the UI delta formatting work, rather than splitting — they're already separate commits and ride the same release window.
Generated by Claude Code
Comment on lines
7
to
+13
| type DeltaProps = { | ||
| pct: number; | ||
| lang: string; | ||
| id?: string; | ||
| }; | ||
|
|
||
| export const Delta: FC<DeltaProps> = ({ pct, id }) => { | ||
| export const Delta: FC<DeltaProps> = ({ pct, lang, id }) => { |
Member
Author
There was a problem hiding this comment.
Keeping lang required here. There are only three Delta callers (dashboard.tsx via KpiCard, bundles.tsx, links.tsx) and lang is already in scope at each — the prop drilling is one line per call site. A silent fallback would mask future i18n bugs (the server has no meaningful "user locale" outside the cookie/settings, so Intl.NumberFormat().resolvedOptions().locale would just return the Worker's runtime locale, not the user's). The matching server-side fmtNumber(n, lang) helper takes lang as required for the same reason.
Generated by Claude Code
| valueId?: string; | ||
| deltaPct?: number | null; | ||
| deltaId?: string; | ||
| lang: string; |
Member
Author
There was a problem hiding this comment.
Same answer as the Delta thread above: KpiCard is only used inside admin pages where lang is already in scope, and a silent fallback would mask future i18n bugs. Keeping lang required so the type system catches a missing locale at the call site rather than rendering en-US-grouped numbers to a Swedish or Indonesian user.
Generated by Claude Code
| valueId, | ||
| deltaPct, | ||
| deltaId, | ||
| lang, |
Member
Author
There was a problem hiding this comment.
| <span>{label}</span> | ||
| </div> | ||
| {deltaPct !== undefined && deltaPct !== null && <Delta pct={deltaPct} id={deltaId} />} | ||
| {deltaPct !== undefined && deltaPct !== null && <Delta pct={deltaPct} lang={lang} id={deltaId} />} |
Member
Author
There was a problem hiding this comment.
The grouping assertion previously relied on the app's default lang being "en". Set the cookie explicitly so the test stays deterministic if the default locale ever changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
src/components/kpi-card.tsx:1
- Making
langrequired on a reusable component is a breaking API change for any existingKpiCardcall sites outside this diff. To keep the change backward-compatible, consider makinglangoptional with a sensible default (or deriving from a shared i18n context) while still allowing callers to override it.
// Copyright 2026 Oddbit (https://oddbit.id)
LICENSE:1
- The stated goal is “canonical Apache-2.0 text from apache.org so scanners recognize the license,” but the updated wording (e.g., dropping “the Licensor”) may diverge from the exact canonical text; scanners can be strict about exact matches. Suggest replacing the entire file contents by copying verbatim from the authoritative source (including punctuation/spacing/wording) to ensure license-detection tools recognize it reliably.
LICENSE:1 - The stated goal is “canonical Apache-2.0 text from apache.org so scanners recognize the license,” but the updated wording (e.g., dropping “the Licensor”) may diverge from the exact canonical text; scanners can be strict about exact matches. Suggest replacing the entire file contents by copying verbatim from the authoritative source (including punctuation/spacing/wording) to ensure license-detection tools recognize it reliably.
LICENSE:1 - The stated goal is “canonical Apache-2.0 text from apache.org so scanners recognize the license,” but the updated wording (e.g., dropping “the Licensor”) may diverge from the exact canonical text; scanners can be strict about exact matches. Suggest replacing the entire file contents by copying verbatim from the authoritative source (including punctuation/spacing/wording) to ensure license-detection tools recognize it reliably.
LICENSE:1 - The stated goal is “canonical Apache-2.0 text from apache.org so scanners recognize the license,” but the updated wording (e.g., dropping “the Licensor”) may diverge from the exact canonical text; scanners can be strict about exact matches. Suggest replacing the entire file contents by copying verbatim from the authoritative source (including punctuation/spacing/wording) to ensure license-detection tools recognize it reliably.
Math.round can return -0 for values like ((99.9 - 100) / 100) * 100, and Intl.NumberFormat renders -0 as "-0" (or U+2212-prefixed in sv). With the old comparison logic, dir lands at "flat" while the label shows "-0%". Add `pct + 0` to coerce -0 → +0 before computing direction, sign, and formatted output. New unit tests cover the regression along with the existing positive/negative/locale-grouping cases.
| valueId?: string; | ||
| deltaPct?: number | null; | ||
| deltaId?: string; | ||
| lang: string; |
Member
Author
There was a problem hiding this comment.
Settled in the prior kpi-card.tsx:16 thread above — keeping lang required by design so the type system catches a missing locale at the call site (admin-only callers, no meaningful Worker runtime locale to fall back to).
…e zero The previous `pct + 0` form produced the same result but read like an accidental coercion. The explicit `Object.is(pct, -0) ? 0 : pct` makes the intent obvious to future readers.
The 1400% delta test prepared the same INSERT inside the loop. Prepare once and rebind per iteration.
…parators # Conflicts: # sdk/dart/CHANGELOG.md # sdk/dart/pubspec.yaml
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/components/kpi-card.tsx:1
- Making
langrequired onKpiCardis a breaking public API change for any existing call sites that don’t have a locale readily available. Consider makinglangoptional with a documented default (e.g., default to"en"in the component parameter destructuring), or derive it from a shared i18n context if one exists, to keep the component easier to consume.
// Copyright 2026 Oddbit (https://oddbit.id)
LICENSE:1
- The PR description mentions “Bump Dart SDK to
1.0.2and add a CHANGELOG entry”, but those changes aren’t shown in the provided diffs. Either include the Dart SDK/CHANGELOG updates in this PR or adjust the PR description to match what’s actually being shipped.
Comment on lines
7
to
+19
| type DeltaProps = { | ||
| pct: number; | ||
| lang: string; | ||
| id?: string; | ||
| }; | ||
|
|
||
| export const Delta: FC<DeltaProps> = ({ pct, id }) => { | ||
| const dir = pct > 0 ? "up" : pct < 0 ? "down" : "flat"; | ||
| export const Delta: FC<DeltaProps> = ({ pct, lang, id }) => { | ||
| // Normalize -0 to 0 so Intl.NumberFormat does not render a confusing "-0%" | ||
| // when dir is "flat". | ||
| const safePct = Object.is(pct, -0) ? 0 : pct; | ||
| const dir = safePct > 0 ? "up" : safePct < 0 ? "down" : "flat"; | ||
| const icon = dir === "up" ? "trending_up" : dir === "down" ? "trending_down" : "trending_flat"; | ||
| const sign = pct > 0 ? "+" : ""; | ||
| const sign = safePct > 0 ? "+" : ""; |
| <span class={`delta ${dir}`} id={id} data-delta={String(safePct)}> | ||
| <span class="icon">{icon}</span> | ||
| <span class="delta-label">{sign}{pct}%</span> | ||
| <span class="delta-label">{sign}{fmtNumber(safePct, lang)}%</span> |
Reverts the text changes from 06645da. Restores the verbatim apache.org wording ("submitted to Licensor", "received by Licensor", "excluding those notices") and the APPENDIX block at the end of the file.
The previous revert was wrong: main carried older non-canonical wording, and 06645da's text was the apache.org canonical form. Verified byte-for-byte against https://www.apache.org/licenses/LICENSE-2.0.txt.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UI:
Deltacomponent now formatspctthroughfmtNumber(pct, lang)so a 4-digit delta renders as+1,400%(en) instead of+1400%.KpiCardacceptslangand forwards it toDelta. Every call site (dashboard.tsxfour KPIs,bundles.tsxcard delta,links.tsxrow delta) passes the page'slangthrough.fmtNumberusesIntl.NumberFormatso groupings follow the active language.Licensing / packaging (folded in to ride the same release window):
LICENSEfiles with the canonical Apache-2.0 text from apache.org so pana/license scanners recognize the license.1.0.2and add a CHANGELOG entry so pub.dev re-scores against the canonical LICENSE.Test plan
yarn test— 866 passedlinks-page.test.ts: a delta of+1400%must surface as+1,400%and never as the bare form. Test pinsCookie: lang=enso the grouping assertion is deterministic.